30 ms is below the minimum required for root ports (which is what we are dealing with here). The relevant excerpt here is:

As an additional requirement, Host Controllers and the USB System Software must ensure that resets issued to the root ports drive reset long enough to overwhelm any concurrent resume attempts by downstream devices. It is required that resets from root ports have a duration of at least 50 ms (TDRSTR). It is not required that this be 50 ms of continuous Reset signaling. However, if the reset is not continuous, the interval(s) between reset signaling must be less than 3 ms (TRHRSI), and the duration of each SE0 assertion must be at least 10 ms (TDRST).

Oh, this is very correct note.

priv->has_device = 1; is set even if speed is not supported. Perhaps creating a copy of each in each case?

LOG here is accessing speed value before check if it is valid. What if you add in each case the proper log like:

LOG_DBG("New device, Low-speed");
LOG_DBG("New device, Full-speed");
LOG_DBG("New device, High-speed");

That was the log message when I developed the driver.

But to be honest, it doesn't make a lot of sense to keep it any longer so we can simplify if just to print the register value of speed at low level drive and remove the uhc_dwc2_speed_str completely.

Also, keep the speed parsing log to the higher layer usbh.

In PR description, you wrote that building needs -DCONFIG_UDC_DWC2=n. If that is the case, why not removing default y from here?

What about simply add depends on !CONFIG_UDC_DWC2 line for the time being? This could be removed once (if at all) there is dynamic role switching implemented.

According to the uhc driver implementation, there is not strict dependency from UDC and it is possible to have both at the same time.
For example, there should be no problem in having both drivers complied, but not running at the same time. That means, we can switch between them, via uhc_init, uhc_enable, then uhc_disable, uhc_shutdown and then udc_init and udc_enable (but uhc_shutdown is not implemented yet).

I added the -DCONFIG_UDC_DWC2=n to the build command at a very early stage, when there was a conflict during the build (multiple definition of `__device_dts_ord_75').

Right now I know a bit more about the environment so I'll double check the conflict, maybe it is possible to make it right. But if it is a DT- conflict in utilizing the same peripheral (usb_otg), then let me know.

UPD: OK, seems that it is technically impossible. So seems that right now it gives us only two options: make them mutually exclusive or assign the node to UDC/UHC via menuconfig.

I'll think about it a bit more, but let me know if there are any preferences.

It might only have been a problem before this was merged (which required manually turning off the device mode): #105005

As enabling host/device is still happening manually so it might work to enable both by default, and the user would then be responsible to enable host or device stack.

If we enable both at the same time - we get the multiple definition of the node during the build.

I don't think (please, correct me if I am missing something) that changing the device tree description to having the assign of the role there something like this does make sense:

usb_otg: usb@40080000 {
    compatible = "snps,dwc2";
    reg = <0x40080000 0x40000>;
    interrupts = <...>;
    zephyr,usb-role = "host";
    status = "okay";
};

What about simply add depends on !CONFIG_UDC_DWC2 line for the time being?

So, I think that simple mutual exclusive like above is the best option at the moment.

The devicetree is there to describe what exists in the hardware, and things like "role" is how this hardware is used by drivers, which I think is a different concern.

If a device needs multiple drivers (i.e. host and device mode), indeed it needs multiple devicetree nodes, such as having child nodes, one for device and one for host, both always enabled and Kconfig used to select what to leverage, as usual?

Though this seems a long-term issue, not needing a solution on this PR? Still glad to discuss it.

missing static here.

typo: Assign

Should you loop over the channels and release it all in here?

for(...)
uhc_dwc2_channel_release(dev, ch);
uhc_xfer_return(dev, xfer, -ENODEV);

Same to the case of UHC_DWC2_EVENT_PORT_ERROR below..

channel errors will be handled in another PR?

That depends on several things now.

I am proceeding with the ztest for uhc at the moment, so I can verify as many possible scenarios as I can and decrease the regression during the future changes.

Currently, the goal of this PR is to unlock the vendors to accomplish the quirks part if this is needed. Current state allows to test the peripheral and interact with USB device if there are no errors and USB device doesn't require any specific handling.

So, if till the moment I finish the tests, add all missing parts and close all TODOs before this PR is merged - I can include them in here as well.

Insight from the testing

Low speed devices doesn't work, because of FrInt wrong configuration for FSLS PHY.

There is not need to split configuration of HCFG and HFIR registers, because they control the same thing: host timings based on the port speed and PHY clock.

In this case, it is better to merge the configuration and provide something like dwc2_hal_config_timings and do that after port is enabled.

Information about the PHY might be gathered during dwc2_hal_init_gusbcfg and being kept in struct { bool is_hs; uint32_t clock_mhz; } phy; instead of simple uint32_t phy_clock_mhz;.

That will allowed to config timing in one place and both regosters together (HCFG and HFIR) avoiding the possible misconfiguration for different speed (such as for Low speed with FSLS PHY in the current code).

I don't quite grasp what you mean by bool is_hs. Do you mean there is need for dynamically switching the PHY used based on device connected?

UTMI+ PHY can just operate fine in either 30 MHz (for 16-bit) or 60 MHz (for 8-bit) mode regardless of what speed is the device connected. This does apply even when CONFIG_UHC_DRIVER_HIGH_SPEED_SUPPORT_ENABLED=n because then FSLSSupp would simply be 1 instead of 0. On nRF54LM20 I believe FSLSPclkSel should always be CLK3060 (0).

Do you mean there is need for dynamically switching the PHY used based on device connected?

No, not dynamically.
This is a flag is_hs = usb_dwc2_get_ghwcfg2_hsphytype(ghwcfg2) != 0.
It is possible to remove it and read the ghwcfg2 instead.

And then, after the port is enabled, something like this:

        case USB_DWC2_HPRT_PRTSPD_HIGH:
		fslspclk = USB_DWC2_HCFG_FSLSPCLKSEL_CLK3060;
		frint = 125U * phy->clock_mhz - 1;
		break;

	case USB_DWC2_HPRT_PRTSPD_FULL:
		if (usb_dwc2_get_ghwcfg2_hsphytype(ghwcfg2) == 0) {
			fslspclk = USB_DWC2_HCFG_FSLSPCLKSEL_CLK48;
		} else {
			fslspclk = USB_DWC2_HCFG_FSLSPCLKSEL_CLK3060;
		}
		frint = 1000U * phy->clock_mhz - 1U;
		break;

	case USB_DWC2_HPRT_PRTSPD_LOW:
		fslspclk = USB_DWC2_HCFG_FSLSPCLKSEL_CLK6;
		frint = 1000U * 6U - 1U;
		break;

PS: CLK6 for Low speed is right for esp32, but it might be not right for nrf54.

UPD: I just tried the changes with FS and LS devices on the test (50 enumeration cycles) and yes it is possible to simplify it by removing the is_hs flag.
I will double check the nrf54 part, because it is not possible to enable/disable driver there to imitate disconnection, but once I am there I can run the tests on nrf54 as well.

ICYAI, I am talking about this tests: roma-jam-lab#7

I think the code should rather be something like:

if (usb_dwc2_get_ghwcfg2_hsphytype(ghwcfg2) == USB_DWC2_GHWCFG2_HSPHYTYPE_NO_HS) {
  set fslspclk and clock_mhz based on port type
} else {
  fslspclk = USB_DWC2_HCFG_FSLSPCLKSEL_CLK3060;
  set clock_mhz to 30 Mhz if USB_DWC2_GUSBCFG_PHYIF_16_BIT else 60 Mhz
}

And then a switch for port type to determine frint value.

I will double check the nrf54 part, because it is not possible to enable/disable driver there to imitate disconnection, but once I am there I can run the tests on nrf54 as well.

What do you mean by "to imitate disconnection"? Do you just want to disable the host pulldowns?

I think the code should rather be something like

This might be another option.

Currently, the initialization and configuration has been split in two parts:

• initialization during the uhc_init(), when we configure the controller based on the used PHY and kept the PHY parameters (only clock is valid so far) till the port will be enabled
• configuration after port was enabled. Here we apply the necessary config, based on the PHY clock and port speed.

I did that, because PHY can't be changed after init and we need to configure the registers as quick as possible. For me it seems that it fits your idea. Let me know if it doesn't.

What do you mean by "to imitate disconnection"?

Right now I use uhc_disable() to disable the controller, which turns of the port power and might be used as a simplified analogue for physical disconnection.

Hi @tmon-nordic,

it is not directly related, but thanks for the hint!
I checked the code and now Low-speed devices on nRF54LM20 are also working.

I will double check the changed code again, refactor it a bit, verify with tests and include in this PR during the next update iteration.